Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let some workers implement AutoCloseable #1786

Merged
merged 3 commits into from
Mar 17, 2022

Conversation

lefou
Copy link
Member

@lefou lefou commented Mar 16, 2022

  • Let some workers implement AutoCloseable
  • Let VizWorker extends AutoCloseable

@lolgab
Copy link
Member

lolgab commented Mar 17, 2022

Very nice! Do you expect to have visible performance improvements from this?

@lefou
Copy link
Member Author

lefou commented Mar 17, 2022

Very nice! Do you expect to have visible performance improvements from this?

Not at all. But I care about resources (which tend to blow up in completely unrelated situations, and is hard to debug) and mill server processes can last very long, so it may make some difference. It's more the fact that we now support resource cleanup, and Module (worker) authors will look into existing code as a guide how to implement their stuff. If they see sme cleanup code, they hopefully learn that it is something worth thinking about.

@lefou
Copy link
Member Author

lefou commented Mar 17, 2022

@lolgab Do you have an idea how we can implement the VizWorker cleanup in a binary compatible way? It's probably the only worker really worth to cleanup, as it starts (and never stops) a background thread. Or do you think we can make an exception and just ignore the binary breakage here? See my second commit as a first attempt.

@lolgab
Copy link
Member

lolgab commented Mar 17, 2022

The only idea I have is to define a new vizWorker and deprecate worker. In 0.11 we can delete worker and in 0.12 we can rename vizWorker back to worker.

You can extract the implementation of the worker in a private function.

@lefou
Copy link
Member Author

lefou commented Mar 17, 2022

Ok, I will address the VisualizeModule.worker in a different PR.

@lefou lefou requested a review from lolgab March 17, 2022 09:50
@lefou lefou merged commit 3ccc0d0 into com-lihaoyi:main Mar 17, 2022
@lefou lefou deleted the closable-workers branch March 17, 2022 10:44
@lefou lefou added this to the after 0.10.1 milestone Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants